Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Input::remove_joy_mapping #98792

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

MJacred
Copy link
Contributor

@MJacred MJacred commented Nov 3, 2024

Erasing a joypad mapping can invalidate other attached joypads and the fallback mapping guid

Fixes #91257 (cannot confirm crash on Linux. Untested on Windows where it was observed)

If this PR is merged, it needs to be cherry-picked/backported to all supported Godot versions, afaik.

NOTE

  • I'm not sure if I handled fallback_mapping correctly, because according to Input::is_joy_known the fallback does not count as a known device… Therefore, feedback on this would be nice
    • If it's only meant as an internal fallback, a more minimal change of this PR to the fallback logic would be to deny removing the mapping for that one. Then I could remove the setter and getter for the fallback. -> Removed bindings
  • I renamed some members in windows joypad file to differentiate between direct INPUT and direct X. I found d_joypads, and it feels like the d_ prefix was meant as a direct input abbreviation. So I used that. I'm open to alternative naming convention, though. Some renaming should happen to untangle INPUT and X. -> Renaming is done in another PR

@MJacred MJacred requested review from a team as code owners November 3, 2024 11:42
@MJacred MJacred changed the title Fix Input::remove_joy_mapping Draft: Fix Input::remove_joy_mapping Nov 3, 2024
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from 0c33bde to 42f5514 Compare November 3, 2024 11:51
@MJacred MJacred changed the title Draft: Fix Input::remove_joy_mapping Fix Input::remove_joy_mapping Nov 3, 2024
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from 42f5514 to fd8aa95 Compare November 3, 2024 12:02
@AThousandShips AThousandShips added bug topic:input crash cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 3, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 3, 2024
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from fd8aa95 to ce6fae4 Compare November 3, 2024 15:28
@MJacred MJacred requested a review from a team as a code owner November 3, 2024 15:28
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from ce6fae4 to 556f90b Compare December 14, 2024 11:37
doc/classes/Input.xml Outdated Show resolved Hide resolved
doc/classes/Input.xml Outdated Show resolved Hide resolved
doc/classes/Input.xml Outdated Show resolved Hide resolved
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from 8a92f94 to 18bce29 Compare December 15, 2024 12:39
Erasing a joypad mapping can invalidate other attached joypads and the fallback mapping guid
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from 18bce29 to 4083238 Compare December 16, 2024 19:27
@MJacred
Copy link
Contributor Author

MJacred commented Dec 16, 2024

status update:

  • Rebased PR to latest master, resolving merge conflict.
  • Removed bindings in Godot's spirit of "don't give bindings to things people don't need"

@akien-mga
Copy link
Member

CC @Rindbee if you can review this as you're also familiar with that code after #95486.

Copy link
Contributor

@Rindbee Rindbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it easier and more intuitive to record the old index than to calculate it. There is nothing wrong with the logic.

core/input/input.cpp Outdated Show resolved Hide resolved
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from 086a11b to f7c6a86 Compare January 13, 2025 10:43
@MJacred MJacred requested a review from Rindbee January 13, 2025 10:48
core/input/input.cpp Outdated Show resolved Hide resolved
core/input/input.cpp Outdated Show resolved Hide resolved
core/input/input.cpp Outdated Show resolved Hide resolved
core/input/input.cpp Show resolved Hide resolved
core/input/input.cpp Outdated Show resolved Hide resolved
core/input/input.cpp Show resolved Hide resolved
@Rindbee
Copy link
Contributor

Rindbee commented Jan 13, 2025

@MJacred Things are more complicated than I initially thought. Especially in the case of being between the minimum removal index and the maximum removal index. For example, suppose that more than three indexes are removed, and there is another guid between the minimum removed index and the maximum removed index, and the guid has multiple mipping records.

--------r0--ai-----r1-aj---r2----r3------

Using simple variables is not enough to record these shifts. To do this, we may need to record all of the mapping indexes of the p_guid.

@MJacred
Copy link
Contributor Author

MJacred commented Jan 13, 2025

@Rindbee: I think I got it.

Alternatively, I can iterate over joy_names in the first for-loop, right where I remove the entry and fix the offset for each affected joypad right then and there

EDIT: fixed syntax check failure and typo

EDIT 2: did a comparison with original commit, and the differences are

  • early exit if nothing removed
  • use fallback_mapping instead of setting to -1 for joypads that used this guid and which was not the fallback mapping
  • simplified joypad mapping fix if (count == 1) (which should be the case 99% of the time)
    • else, the old inner-loop is back again
  • 2 new helper variables allowing the early exit and skipping unnecessary mapping fixings

@MJacred MJacred force-pushed the fix_remove_joy_mapping branch 2 times, most recently from ae692f3 to cd0bc6e Compare January 13, 2025 15:43
@MJacred MJacred requested a review from Rindbee January 13, 2025 15:54
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from cd0bc6e to f355382 Compare January 13, 2025 15:55
core/input/input.cpp Outdated Show resolved Hide resolved
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from 92b860a to b5012c7 Compare January 14, 2025 10:43
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from b5012c7 to 8e75fae Compare January 14, 2025 11:09
@MJacred MJacred requested a review from Rindbee January 17, 2025 12:11
Copy link
Contributor

@Rindbee Rindbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no problem with the logic and it is relatively clear.

@Repiteo Repiteo merged commit 49481c1 into godotengine:master Jan 17, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 17, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release crash topic:input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove a non-xinput device joymapping cause game crash
7 participants